Skip to content

Fix Caching strategy on form_post html#2294

Merged
kevinchalet merged 3 commits intoopeniddict:devfrom
matthid:no-store
Apr 11, 2025
Merged

Fix Caching strategy on form_post html#2294
kevinchalet merged 3 commits intoopeniddict:devfrom
matthid:no-store

Conversation

@matthid
Copy link
Contributor

@matthid matthid commented Apr 10, 2025

While this is kind of minor, we still wanted to fix this caching issue and contribute it back to here.
In the mean time we already changed to a custom handler.

This issue was found in an audit of our system.

@matthid
Update OpenIddictServerAspNetCoreHandlers.Authentication.cs
a74c8e1 
Fix Caching strategy on form_post html
@kevinchalet
Copy link
Member

kevinchalet commented Apr 10, 2025

Thanks for your PR!

The Cache-Control: no-cache value was inherited from Katana/OWIN's OAuth 2.0 authorization server middleware: https://github.com/aspnet/AspNetKatana/blob/86fa511a6c4a598b08cf13a62280783bf2ea472f/src/Microsoft.Owin.Security.OAuth/OAuthAuthorizationServerHandler.cs#L475-L479. I vaguely remember it was discussed with the Katana team many years ago, but I can't find the thread (maybe it was on Codeplex?) and I don't remember the outcome of that discussion 😅

That said, we'll definitely want to change it since the OAuth 2.0 specification explicitly require using no-store:

The authorization server MUST include the HTTP "Cache-Control"
response header field [[RFC2616](https://datatracker.ietf.org/doc/html/rfc2616)] with a value of "no-store" in any
response containing tokens, credentials, or other sensitive
information, as well as the "Pragma" response header field [[RFC2616](https://datatracker.ietf.org/doc/html/rfc2616)]
with a value of "no-cache".

https://datatracker.ietf.org/doc/html/rfc6749#section-5.1

Interestingly, the "OAuth 2.0 Form Post Response Mode" specification includes a (non-normative) appendix that uses Cache-Control: no-cache, no-store instead of Cache-Control: no-cache, which isn't compliant with the original OAuth 2.0 specification (it wouldn't be the first time, tho' 😄)

After authentication and approval by the End-User, the Authorization Server issues the Authorization Response:

HTTP/1.1 200 OK
Content-Type: text/html;charset=UTF-8
Cache-Control: no-cache, no-store
Pragma: no-cache

<html>
  <head><title>Submit This Form</title></head>
  <body onload="javascript:document.forms[0].submit()">
    <form method="post" action="https://client.example.org/callback">
      <input type="hidden" name="state"
       value="DcP7csa3hMlvybERqcieLHrRzKBra"/>
      <input type="hidden" name="id_token"
       value="eyJhbGciOiJSUzI1NiIsImtpZCI6IjEifQ.eyJzdWIiOiJqb2huIiw
         iYXVkIjoiZmZzMiIsImp0aSI6ImhwQUI3RDBNbEo0c2YzVFR2cllxUkIiLC
         Jpc3MiOiJodHRwczpcL1wvbG9jYWxob3N0OjkwMzEiLCJpYXQiOjEzNjM5M
         DMxMTMsImV4cCI6MTM2MzkwMzcxMywibm9uY2UiOiIyVDFBZ2FlUlRHVE1B
         SnllRE1OOUlKYmdpVUciLCJhY3IiOiJ1cm46b2FzaXM6bmFtZXM6dGM6U0F
         NTDoyLjA6YWM6Y2xhc3NlczpQYXNzd29yZCIsImF1dGhfdGltZSI6MTM2Mz
         kwMDg5NH0.c9emvFayy-YJnO0kxUNQqeAoYu7sjlyulRSNrru1ySZs2qwqq
         wwq-Qk7LFd3iGYeUWrfjZkmyXeKKs_OtZ2tI2QQqJpcfrpAuiNuEHII-_fk
         IufbGNT_rfHUcY3tGGKxcvZO9uvgKgX9Vs1v04UaCOUfxRjSVlumE6fWGcq
         XVEKhtPadj1elk3r4zkoNt9vjUQt9NGdm1OvaZ2ONprCErBbXf1eJb4NW_h
         nrQ5IKXuNsQ1g9ccT5DMtZSwgDFwsHMDWMPFGax5Lw6ogjwJ4AQDrhzNCFc
         0uVAwBBb772-86HpAkGWAKOK-wTC6ErRTcESRdNRe0iKb47XRXaoz5acA"/>
    </form>
  </body>
</html>

https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html#FormPostResponseExample

So, for compliance and consistency with the other code paths that also return the Cache-Control, we should probably use no-store instead of no-cache, no-store. We'll also want to update the OWIN handler that does the same thing:

openiddict-core/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Authentication.cs

Lines 203 to 205 in 03e1b28

response.Headers[Headers.CacheControl] = "no-cache";
response.Headers[Headers.Pragma] = "no-cache";
response.Headers[Headers.Expires] = "-1";

@matthid
Copy link
Contributor Author

matthid commented Apr 11, 2025 •
edited
Loading

"MUST include" I think there is some room for interpretation here:
https://stackoverflow.com/questions/3241326/set-more-than-one-http-header-with-the-same-name
points to https://www.rfc-editor.org/rfc/rfc7230#section-3.2.2

A recipient MAY combine multiple header fields with the same field
name into one "field-name: field-value" pair, without changing the
semantics of the message, by appending each subsequent field value to
the combined field value in order, separated by a comma. The order
in which header fields with the same field name are received is
therefore significant to the interpretation of the combined field
value; a proxy MUST NOT change the order of these field values when
forwarding a message.

Hence one could argue that Cache-Control: no-store, no cache is equivalent to

Cache-Control: no-store
Cache-Control: no-cache

Hence it is included as required.

Do you take this from here and change as required? Since it is such a simple change I'd argue it is not worth the back and forth. Just change it, no attribution required ;)

PS: Thanks for the fast and deep research on this, good job!

@kevinchalet
Copy link
Member

kevinchalet commented Apr 11, 2025 •
edited
Loading

Hence it is included as required.

Yeah, it's not unreasonable, tho' it's not something the normative sample mentioned in the OAuth 2.0 specification does (e.g for the token response):

HTTP/1.1 200 OK
Content-Type: application/json;charset=UTF-8
Cache-Control: no-store
Pragma: no-cache

{
  "access_token":"2YotnFZFEjr1zCsicMWpAA",
  "token_type":"example",
  "expires_in":3600,
  "refresh_token":"tGzv3JOkF0XG5Qx2TlKWIA",
  "example_parameter":"example_value"
}

Same thing in the OIDC base specification:

HTTP/1.1 200 OK
Content-Type: application/json
Cache-Control: no-store

{
   "access_token": "SlAV32hkKG",
   "token_type": "Bearer",
   "refresh_token": "8xLOxBtZp8",
   "expires_in": 3600,
   "id_token": "eyJhbGciOiJSUzI1NiIsImtpZCI6IjFlOWdkazcifQ.ewogImlzc
     yI6ICJodHRwOi8vc2VydmVyLmV4YW1wbGUuY29tIiwKICJzdWIiOiAiMjQ4Mjg5
     NzYxMDAxIiwKICJhdWQiOiAiczZCaGRSa3F0MyIsCiAibm9uY2UiOiAibi0wUzZ
     fV3pBMk1qIiwKICJleHAiOiAxMzExMjgxOTcwLAogImlhdCI6IDEzMTEyODA5Nz
     AKfQ.ggW8hZ1EuVLuxNuuIJKX_V8a_OMXzR0EHR9R6jgdqrOOF4daGU96Sr_P6q
     Jp6IcmD3HP99Obi1PRs-cwh3LO-p146waJ8IhehcwL7F09JdijmBqkvPeB2T9CJ
     NqeGpe-gccMg4vfKjkM8FcGvnzZUN4_KSP0aAp1tOJ1zZwgjxqGByKHiOtX7Tpd
     QyHE5lcMiKPXfEIQILVq0pc_E2DzL7emopWoaoZTF_m0_N0YzFC6g6EJbOEoRoS
     K5hoDalrcvRYLSrQAZZKflyuVCyixEoV9GfNQC3_osjzw2PAithfubEEBLuVVk4
     XUVrWOLrLl0nx7RkKU8NXNHq-rvKMzqg"
}

https://openid.net/specs/openid-connect-core-1_0.html#TokenResponse

I guess it would be safer not to be too creative here and just use Cache-Control: no-store? 😄

Do you take this from here and change as required? Since it is such a simple change I'd argue it is not worth the back and forth. Just change it, no attribution required ;)

Haha nah, let's continue with your PR: as you said the change should be minimal and it's always better when proper credits are attributed! 👍🏻

@kevinchalet kevinchalet self-assigned this Apr 11, 2025
@kevinchalet kevinchalet added this to the 7.0.0-preview2 milestone Apr 11, 2025
@matthid
Copy link
Contributor Author

matthid commented Apr 11, 2025

There you go ;)

@kevinchalet kevinchalet merged commit 524ce1f into openiddict:dev Apr 11, 2025
6 checks passed
@kevinchalet
Copy link
Member

kevinchalet commented Apr 11, 2025

Merged! Thanks for your contribution 😃

(I'll likely backport this change to the 6.x branch and make sure it's included in the 6.3 release).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@kevinchalet kevinchalet

Labels

bug community contribution server stack

Projects

None yet

Milestone

7.0.0-preview2

Development

Successfully merging this pull request may close these issues.

2 participants

@matthid @kevinchalet